Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement Autostart Feature on Profiler / Visual Profiler / Network Profiler #70766

Closed
wants to merge 8 commits into from
Closed

Implement Autostart Feature on Profiler / Visual Profiler / Network Profiler #70766

wants to merge 8 commits into from

Conversation

stmSi
Copy link
Contributor

@stmSi stmSi commented Dec 31, 2022 •
edited
Loading

proposal: #6017

This also fix for bug where "Network Profiler" Stop Button not updating to Start but the running program was already closed. just like this bug #70143

2022-12-31-20-46-19

@stmSi stmSi requested review from a team as code owners December 31, 2022 10:04
@stmSi stmSi force-pushed the implement-autostart-profilers branch 2 times, most recently from 84eb748 to 095a4f3 Compare December 31, 2022 10:27
@stmSi stmSi requested a review from a team as a code owner December 31, 2022 10:27
@stmSi stmSi force-pushed the implement-autostart-profilers branch 3 times, most recently from f3a505c to ab4bd92 Compare December 31, 2022 14:05
@Calinou Calinou added this to the 4.x milestone Dec 31, 2022
@stmSi stmSi mentioned this pull request Mar 2, 2023
Closed
@stmSi stmSi force-pushed the implement-autostart-profilers branch from ab4bd92 to a4e9ad6 Compare March 2, 2023 08:46
Calinou
Calinou reviewed Mar 3, 2023
View reviewed changes
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me from a cursory glance, although there are some stylistic changes to make (see review comments).

@stmSi stmSi force-pushed the implement-autostart-profilers branch from a4e9ad6 to d116bd2 Compare March 3, 2023 02:49
@stmSi
Copy link
Contributor Author

stmSi commented Mar 3, 2023 •
edited
Loading

@Calinou Updated. Thanks for the review.

@stmSi stmSi force-pushed the implement-autostart-profilers branch 2 times, most recently from 71c4432 to bbc1818 Compare April 7, 2023 11:00
@phil-hudson
Copy link
Contributor

Could this possibly be scheduled for 4.2?

@phil-hudson phil-hudson mentioned this pull request Sep 30, 2023
Closed
@Calinou
Copy link
Member

Calinou commented Oct 1, 2023 •
edited
Loading

Could this possibly be scheduled for 4.2?

This pull request needs a rebase first. Also, feature freeze is in a few days, so it's not guaranteed that it can be finalized and merged before feature freeze occurs.

@stmSi Please rebase this PR if you have time 🙂
If you can't rebase it, let us know so we can mark this PR as salvageable.

@stmSi stmSi force-pushed the implement-autostart-profilers branch from bbc1818 to 2a4f48b Compare October 1, 2023 07:05
@stmSi
Copy link
Contributor Author

stmSi commented Oct 1, 2023

@Calinou I rebased the PR.. ready to review.

Calinou
Calinou requested changes Oct 2, 2023
View reviewed changes
Copy link
Member

@Calinou Calinou left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, it works as expected.

I noticed two issues with the feature's design though:

  • The Autostart button is on the left in the Network Profiler tab, but on the right in the Profiler and Visual Profiler tabs. This should be made consistent across all 3 tabs.
    • I suggest putting Autostart on the left in all 3, so that it's close to the Start button.
  • The setting is stored as a project setting, not an editor setting or per-project metadata. This means the value is persisted to version control, even though it's not relevant for other team members in a project (and can cause noisy diffs to be committed to version control).
    • I suggest using project metadata instead, which isn't committed to version control but is still defined on a per-project basis. You can use EditorSettings::get_singleton()->set_project_metadata() for this purpose.

@stmSi
Copy link
Contributor Author

stmSi commented Mar 18, 2024

@Calinou , Sorry I forgot about the changes, working on it.

@stmSi stmSi force-pushed the implement-autostart-profilers branch from 2a4f48b to 468a733 Compare March 18, 2024 21:24
@stmSi
Copy link
Contributor Author

stmSi commented Mar 19, 2024

Hi @Calinou , I made changes according to feedbacks. Could you please review the changes?

Calinou

This comment was marked as outdated.

Sign in to view

@Calinou
Copy link
Member

Calinou commented Apr 4, 2024

Disregard the above review, it's because we merged Ninja recently and I forgot to update my build script so I was running an old binary.

@stmSi stmSi force-pushed the implement-autostart-profilers branch from a2259f6 to 768c8c1 Compare April 4, 2024 15:37
@stmSi
Copy link
Contributor Author

stmSi commented Apr 4, 2024

I just rebased to master and tested again.

Calinou
Calinou reviewed Apr 4, 2024
View reviewed changes
Copy link
Member

@Calinou Calinou left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, it works as expected.

My only remaining concern is about the Auto Start button: why does it have its focus released as soon as you click it? The other buttons like Start and Clear don't do this in their respective profiler.

simplescreenrecorder-2024-04-04_18.43.57.mp4

@stmSi
Copy link
Contributor Author

stmSi commented Apr 4, 2024 •
edited
Loading

No particular reason. During my test, I just didn't like border focus on the button.. xd .
I will remove the release_focus.

stmSi and others added 4 commits April 5, 2024 02:05
@stmSi @Calinou
Update modules/multiplayer/editor/editor_network_profiler.cpp
4a001b7 
Co-authored-by: Hugo Locurcio <hugo.locurcio@hugo.pro>
@stmSi @Calinou
Update editor/debugger/editor_visual_profiler.cpp
18e2f14 
Co-authored-by: Hugo Locurcio <hugo.locurcio@hugo.pro>
@stmSi @Calinou
Update editor/debugger/editor_profiler.cpp
5074fb6 
Co-authored-by: Hugo Locurcio <hugo.locurcio@hugo.pro>
@stmSi @Calinou
Update editor/debugger/editor_profiler.cpp
abd7716 
Co-authored-by: Hugo Locurcio <hugo.locurcio@hugo.pro>
@Calinou Calinou mentioned this pull request Apr 4, 2024
Merged
@Geometror
Copy link
Member

Hey @stmSi,
sorry for the long wait. Since this is an important UX enhancement I'd like to help finalize it so we can merge it for 4.4.
Are you still available to do more changes based on my review feedback, or would you prefer that I do the final touches directly myself and add you as a co-author?

@stmSi
Copy link
Contributor Author

stmSi commented Sep 8, 2024

Hi @Geometror , sorry for the late reply. You can take over this PR.

@Geometror Geometror mentioned this pull request Sep 9, 2024
Merged
@stmSi stmSi closed this Sep 11, 2024
@stmSi stmSi deleted the implement-autostart-profilers branch September 11, 2024 03:27
@akien-mga akien-mga removed this from the 4.x milestone Sep 11, 2024
@Calinou Calinou mentioned this pull request Sep 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Calinou Calinou Calinou requested changes

Assignees
No one assigned
Labels
archived enhancement topic:editor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@stmSi @phil-hudson @Calinou @Geometror @akien-mga @tw-sithumyo